Skip to content

Change runDetached to return a ProcessHandle instead of a ProcessIdentifier #95

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: eng/PR-platform-handles
Choose a base branch
from

Conversation

jakepetroules
Copy link
Contributor

@jakepetroules jakepetroules commented Jun 24, 2025

Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns.

This patch now returns a non-copyable ProcessHandle, which provides access to both the ProcessIdentifier and the underlying platform handles (like Execution), while also guaranteeing that the underlying platform handles are released once the ProcessHandle goes out of scope. In effect, the platform handles are now released at the end of the runDetached caller's scope, rather than just before runDetached itself returns to the caller, solving the Windows-specific race condition.

This requires new API because ProcessIdentifier is Hashable and Codable and can't reasonably be made non-Copyable. Similarly, it's likely undesirable to use Execution as the return type because it too would need to be made non-Copyable to prevent the resource leak and there is at least one example in existing tests which requires Execution to be Copyable.

Closes #94

@jakepetroules jakepetroules force-pushed the eng/PR-runDetached-api-eng/PR-windows-pid-reuse-eng/PR-always-monitor branch from e6e13a8 to cebafd9 Compare June 24, 2025 18:20
@jakepetroules
Copy link
Contributor Author

@compnerd Any thoughts on this or the associated GitHub issue?

@compnerd
Copy link
Member

Exposing hProcess is a very powerful construct, so I think that is very useful. I think that this basically is the ultimate escape valve - no matter any functionality that we may miss, the user should be able to workaround that.

I do wonder if we need to worry about the resource leak as the CloseHandle is required whereas on Unix, there is no release for the pid that is returned. Can we use a wrapper type that handles the lifetime and performs a CloseHandle in the case that the reference is lost?

@jakepetroules
Copy link
Contributor Author

I thought about that...

I think we'd need to make Execution non-Copyable and add a deinit that calls CloseHandle? I seem to recall there was previous discussion about doing making Execution non-Copyable, but it didn't happen, maybe @iCharlesHu can provide some info on why that path wasn't chosen at the time?

And then I wonder if we would want some method like "takeHandle" which would allow the client to explicitly take ownership of the HANDLE away from the Execution.

@jakepetroules jakepetroules force-pushed the eng/PR-windows-pid-reuse-eng/PR-always-monitor branch from 7f1597d to 6b7b547 Compare June 27, 2025 18:28
@jakepetroules jakepetroules force-pushed the eng/PR-runDetached-api-eng/PR-windows-pid-reuse-eng/PR-always-monitor branch from cebafd9 to 7ad0121 Compare June 27, 2025 18:30
@iCharlesHu
Copy link
Contributor

I think we should start a discussion about this API change on the Swift forums. The current approach, which involves returning Execution, was actually one of my initial designs. However, it was specifically rejected due to the issue raised by @compnerd: we don’t want to return something and rely on the user to close it. This is the reason why we opted for the closure-based approach.

In my opinion, the only viable way to support this API on Windows is through a with-style closure, allowing us to close the HANDLE after the closure returns. However, this approach defeats the purpose of this API, which was intended to be a straightforward wrapper around the platform spawning API. Consequently, I think we have three options: 1) use the with-style API exclusively on Windows; 2) use the with-style API on all platforms; 3) omit this API on Windows since it’s not safe to offer it. I’m not sure about the most appropriate approach, and given the significant nature of this API change, I think we should discuss it on the forum to gather more feedback.

@jakepetroules
Copy link
Contributor Author

However, it was specifically rejected due to the issue raised by @compnerd: we don’t want to return something and rely on the user to close it.

Doesn't ~Copyable + deinit allow us to close the handle without requiring the user to do so?

Also, I would argue that returning a numeric pid on Unix is still a resource leak of sorts, since the zombie will remain in the process table until the user calls waitid on it. So how's that much different from failing to call CloseHandle on Windows?

@jakepetroules
Copy link
Contributor Author

jakepetroules commented Jun 27, 2025

FWIW, I'm not sure we need runDetached as a dedicated API. I have a need for a long-running process and ended up creating a wrapper class that stores a Task as an ivar, whose body runs the closure based Subprocess API. That should work fine without the need for a dedicated runDetached API.

https://github.com/swiftlang/swift-build/pull/584/files#diff-82ce9e4dd6439d5eba2c40db05433107a2fcf9d7434aa1cec546f0163e755aee

@jakepetroules jakepetroules force-pushed the eng/PR-windows-pid-reuse-eng/PR-always-monitor branch 3 times, most recently from 2b8c265 to 6422350 Compare June 29, 2025 05:34
Base automatically changed from eng/PR-windows-pid-reuse-eng/PR-always-monitor to main June 30, 2025 00:06
@jakepetroules jakepetroules force-pushed the eng/PR-runDetached-api-eng/PR-windows-pid-reuse-eng/PR-always-monitor branch from 7ad0121 to c8c66e1 Compare June 30, 2025 00:24
@jakepetroules jakepetroules changed the base branch from main to eng/PR-platform-handles June 30, 2025 00:25
@jakepetroules
Copy link
Contributor Author

@iCharlesHu I ended up splitting part of this change into #101 (which also introduces new API but is independently useful of these runDetached-specific issues), and now instead of returning Execution, runDetached returns a new non-Copyable type ProcessHandle which guarantees that the underlying platform handle is released (so there can be no resource leaks) while still solving the underlying race condition since the caller now has access to the platform handles from the return value of runDetached.

Happy to still start a thread on the Swift forums if you think it's needed but hopefully this removes the resource leakage controversy in an elegant way!

@FranzBusch
Copy link
Member

FWIW, I'm not sure we need runDetached as a dedicated API. I have a need for a long-running process and ended up creating a wrapper class that stores a Task as an ivar, whose body runs the closure based Subprocess API. That should work fine without the need for a dedicated runDetached API.

FWIW, this was my initial feedback as well. I don't see a need for this API as well since users can just spawn a task to do this detached work. I do recall that @iCharlesHu had a good point for why we need it anyways.

runDetached returns a new non-Copyable type ProcessHandle which guarantees that the underlying platform handle is released

I'm a bit worried about this in general but not too familiar with Windows. The reason we see very little ~Copyable adoption for resources yet is that teardown of most resources is often throwing and/or asynchronous which both are not representable with ~Copyable types. For this reason we normally opt for with-style methods when handling resources.

…tifier

Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns.

This patch now returns a non-copyable ProcessHandle, which provides access to both the ProcessIdentifier and the underlying platform handles (like Execution), while also guaranteeing that the underlying platform handles are released once the ProcessHandle goes out of scope. In effect, the platform handles are now released at the end of the runDetached caller's scope, rather than just before runDetached itself returns to the caller, solving the Windows-specific race condition.

This requires new API because ProcessIdentifier is Hashable and Codable and can't reasonably be made non-Copyable. Similarly, it's likely undesirable to use Execution as the return type because it too would need to be made non-Copyable to prevent the resource leak and there is at least one example in existing tests which requires Execution to be Copyable.

Closes #94
@jakepetroules jakepetroules force-pushed the eng/PR-platform-handles branch from eba918b to e0559da Compare July 1, 2025 00:43
@jakepetroules jakepetroules force-pushed the eng/PR-runDetached-api-eng/PR-windows-pid-reuse-eng/PR-always-monitor branch from c8c66e1 to 14c7b48 Compare July 1, 2025 00:43
@jakepetroules
Copy link
Contributor Author

FWIW, I'm not sure we need runDetached as a dedicated API. I have a need for a long-running process and ended up creating a wrapper class that stores a Task as an ivar, whose body runs the closure based Subprocess API. That should work fine without the need for a dedicated runDetached API.

FWIW, this was my initial feedback as well. I don't see a need for this API as well since users can just spawn a task to do this detached work. I do recall that @iCharlesHu had a good point for why we need it anyways.

runDetached returns a new non-Copyable type ProcessHandle which guarantees that the underlying platform handle is released

I'm a bit worried about this in general but not too familiar with Windows. The reason we see very little ~Copyable adoption for resources yet is that teardown of most resources is often throwing and/or asynchronous which both are not representable with ~Copyable types. For this reason we normally opt for with-style methods when handling resources.

This is a good point. CloseHandle can fail, but should only be in cases where you've given an invalid handle or tried to close the same one multiple times, both of which are programmer error and can be precondition'd. I'm not sure if there could be platforms in the future where this is not the case (Fuschia or something?), but for now any current and speculative API calls in teardown would be CloseHandle on Windows and close on POSIX (for process file descriptors, which Linux and FreeBSD both have).

Indeed if we could simply remove runDetached entirely, that would solve the problem. I'm interested in hearing @iCharlesHu's original reasoning for why we needed it, or if we could instead express whatever that was with the closure-based API.

@jakepetroules jakepetroules changed the title Change runDetached to return an Execution instead of a ProcessIdentifier and make the HANDLE public Change runDetached to return an Execution instead of a ProcessIdentifier Jul 1, 2025
@jakepetroules jakepetroules changed the title Change runDetached to return an Execution instead of a ProcessIdentifier Change runDetached to return a ProcessHandle instead of a ProcessIdentifier Jul 1, 2025
Copy link
Contributor

@iCharlesHu iCharlesHu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API change needs to be discussed.

@itingliu
Copy link
Contributor

itingliu commented Jul 7, 2025

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants